🛡️ Sentinel: [CRITICAL] Fix command injection in docker-logs API#41
🛡️ Sentinel: [CRITICAL] Fix command injection in docker-logs API#41bobdivx wants to merge 1 commit into
Conversation
Co-authored-by: bobdivx <6737167+bobdivx@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical command injection vulnerability in the Docker logs API by replacing execSync with execFileAsync and implementing input validation for the containerId and tail parameters. It also improves security by sanitizing error messages to prevent the leakage of internal application details. Feedback from the review highlights an issue where the log aggregation logic incorrectly uses a logical OR, potentially omitting stderr content. Furthermore, the reviewer pointed out that returning raw stderr in the response body still poses a risk of information exposure, conflicting with the stated security goals.
| // Certains logs sortent sur stderr, fusionnons ou prenons stdout si dispo | ||
| const output = stdout.trim() || stderr.trim(); |
There was a problem hiding this comment.
The code uses the logical OR operator (||), which means if stdout has any content, stderr is completely ignored. This contradicts the comment on line 38 ("fusionnons") and leads to incomplete logs, as many Dockerized applications distribute their output across both streams (e.g., access logs to stdout and error logs to stderr). To ensure all logs are captured, you should concatenate both streams.
| // Certains logs sortent sur stderr, fusionnons ou prenons stdout si dispo | |
| const output = stdout.trim() || stderr.trim(); | |
| // Fusion des flux stdout et stderr pour capturer l'intégralité des logs | |
| const output = (stdout + stderr).trim(); |
| // Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur | ||
| // En cas d'erreur de commande (ex: conteneur introuvable), docker renvoie les infos sur stderr | ||
| if (err.stderr) { | ||
| logs = err.stderr.toString().trim().split('\n'); |
There was a problem hiding this comment.
This line returns the raw stderr output from a failed command directly to the client within the logs array, accompanied by a 200 OK status. This appears to contradict the PR's objective of sanitizing error messages and avoiding the leakage of internal application details (as stated in the PR description and .jules/sentinel.md). If the command fails, it should ideally return a sanitized error response with an appropriate error status code (e.g., 400 or 500).
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The
src/pages/api/docker-logs.tsAPI endpoint usedexecSyncwith unsanitized user input (containerIdandtail), creating a command injection vulnerability. A malicious user could execute arbitrary commands on the host by passing payloads like; whoamior inject arguments via-.🎯 Impact: Full remote code execution (RCE) on the server running the API.
🔧 Fix:
execSyncwithexecFileAsyncto safely execute the command using an argument array, eliminating shell interpretation.-).✅ Verification: Code compiles, tests pass, logic reviewed. Documented learning in
.jules/sentinel.md.PR created automatically by Jules for task 1612493473874286515 started by @bobdivx